Barecheck - Code coverage report

Your code coverage diff: -0.02% ▾

I've skimmed the code and it looks very much like it does what it's supposed to with a minimum of fuss. Having a way to add config that's shared between Things is definitely a feature worth having, though it would be nice if it could be a bit more structured.

My initial thought was to suggest we use a Thing to manage that: the OpenFlexureApplicationThing could then expose the various things needed by everything else, and could be accessed through a thing slot. That keeps everything nicely typed. Unfortunately, at the moment there's no nice way to get information from the configuration file into a Thing, except init_kwargs. So, I suppose that could work, but it wouldn't be beautiful. I also don't think I've got a lot of testing going on for passing arguments to things from the config.

To make that slightly more concrete, how about:

class OFMApplicationThing(lt.Thing):
    def __init__(self, thing_server_interface, **settings):
        super().__init__(thing_server_interface)
        self._settings = **settings

    @property
    def data_folder(self) -> str:
        return self._settings.get("data_folder", "./data")

You could then reference this in the server config as something like

{
    "things": {
        "ofm_app": {
            "class": "path.to:OFMApplicationThing",
            "kwargs": {
                "data_folder": "/var/openflexure/data"
            }
        }
    }
}

One advantage of having a Thing manage the application config is that it gives an obvious place for validation logic, and would even allow settings as well as configuration. I'd be curious to see what you think of a solution like that.

I don't think there's anything fundamentally wrong with having somewhere to pass arbitrary information to Things from the config file - but doing it with a Thing does feel like it makes it easier to manage. I think the implementation in this MR is fine, but I can see it becoming a pain to add in validation nicely.

One thing I have wondered for a while is whether we could come up with a nice way to specify settings in the configuration, i.e. you could add a Thing, but specify the value of some or all of its settings. I think that gets complicated quite quickly: it would be fine for a data setting, and problematic for functional ones. Perhaps what would be helpful here is adding in another descriptor that is a bit like a setting, except it's read-only and set in the config rather than the settings file. Obviously that should be possible at the moment using kwargs when we create the Thing, but it might make the code a bit cleaner.

How validation is done is up to the downstream application. We need to remember that if this is a library that people are going to use in laboratories it needs to also support quick set up, and prototyping, as well as a final locked down application.

If LabThings is too opinionated on everything must be validated then it creating a huge adoption barrier. It also means that any use case that has not been considered becomes very difficult.

I totally agree that downstream code should find a way to validate its information, but that is the responsibility and choice of downstream applications to do what is appropriate for them at that time.

It is also worth noting that information is only available as a thing slot then it isn't available during __init__ or available to other code during setup.

I'm going to assume that @julianstirling you're happy with the tweaks (I guess you can't approve as you made the PR), so I'll merge this now.

I suspect the test coverage decrease is due to the extra code in testing. I should probably add a test_testing module but I think I'll leave that for another day.